[mlir][TosaToLinalg] Ensure valid insertion pointer after op erasure #146908
Conversation
Erasing/replacing an op, which is also the current insertion point, invalidates the insertion point. Explicitly set the insertion point, so that copy does not crash after the One-Shot Dialect Conversion refactoring. (ConversionPatternRewriter will start behaving more like a "normal" rewriter.)
|
@llvm/pr-subscribers-mlir @llvm/pr-subscribers-mlir-linalg Author: Matthias Springer (matthias-springer) ChangesErasing/replacing an op, which is also the current insertion point, invalidates the insertion point. Explicitly set the insertion point, so that subsequent op insertions do not crash after the One-Shot Dialect Conversion refactoring. ( Full diff: https://github.com/llvm/llvm-project/pull/146908.diff 1 Files Affected:
diff --git a/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp b/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
index c460a8bb2f4b2..ca8f6fcb1e122 100644
--- a/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
+++ b/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
@@ -815,6 +815,7 @@ class MaxPool2dConverter : public OpConversionPattern<tosa::MaxPool2dOp> {
ValueRange{paddedInput, fakeWindowDims}, filledEmptyTensor, strideAttr,
dilationAttr);
+ rewriter.setInsertionPointAfter(op);
rewriter.replaceOp(op, resultOp);
// NaN propagation has no meaning for non floating point types.
|
|
@llvm/pr-subscribers-mlir-tosa Author: Matthias Springer (matthias-springer) ChangesErasing/replacing an op, which is also the current insertion point, invalidates the insertion point. Explicitly set the insertion point, so that subsequent op insertions do not crash after the One-Shot Dialect Conversion refactoring. ( Full diff: https://github.com/llvm/llvm-project/pull/146908.diff 1 Files Affected:
diff --git a/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp b/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
index c460a8bb2f4b2..ca8f6fcb1e122 100644
--- a/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
+++ b/mlir/lib/Conversion/TosaToLinalg/TosaToLinalgNamed.cpp
@@ -815,6 +815,7 @@ class MaxPool2dConverter : public OpConversionPattern<tosa::MaxPool2dOp> {
ValueRange{paddedInput, fakeWindowDims}, filledEmptyTensor, strideAttr,
dilationAttr);
+ rewriter.setInsertionPointAfter(op);
rewriter.replaceOp(op, resultOp);
// NaN propagation has no meaning for non floating point types.
|
|
I am curious, do you think it'd be feasible for erase op (and replace op) to accommodate for the insertion point being erased? Eg move the insertion point to the next op if the current is being removed. |
|
I tried this here but it makes op erasure more expensive. What do you think? |
|
Thanks for the fix!
Personally I feel that updating the insertion point should remain the responsibility of the user. Looking at this specific example, it feels like This is just an opinion, I don't have any data for or against a particular approach. |
These insertion points were added in #146551 and #146908 to support the one-shot dialect conversion driver which performs changes to the IR immediately and would otherwise invalidate previous insertion points. Since then, the insertion point has been made resilient against op erasure (#146955) making the changes now redundant.
…837) These insertion points were added in llvm/llvm-project#146551 and llvm/llvm-project#146908 to support the one-shot dialect conversion driver which performs changes to the IR immediately and would otherwise invalidate previous insertion points. Since then, the insertion point has been made resilient against op erasure (llvm/llvm-project#146955) making the changes now redundant.
These insertion points were added in llvm#146551 and llvm#146908 to support the one-shot dialect conversion driver which performs changes to the IR immediately and would otherwise invalidate previous insertion points. Since then, the insertion point has been made resilient against op erasure (llvm#146955) making the changes now redundant.
Erasing/replacing an op, which is also the current insertion point, invalidates the insertion point. Explicitly set the insertion point, so that subsequent op insertions do not crash after the One-Shot Dialect Conversion refactoring. (
ConversionPatternRewriterwill start behaving more like a "normal" rewriter.)